Skip to content

Make golden proof test data available to other languages#171

Merged
AlCutter merged 21 commits intotransparency-dev:mainfrom
tannaurus:tg/write-golden-files
Apr 17, 2025
Merged

Make golden proof test data available to other languages#171
AlCutter merged 21 commits intotransparency-dev:mainfrom
tannaurus:tg/write-golden-files

Conversation

@tannaurus
Copy link
Copy Markdown
Contributor

@tannaurus tannaurus commented Apr 15, 2025

Summary

I created a new proofgen binary writes all of the test cases into json files. The verify_go tests have been updated to walk the file tree and use each json file as input.

Thought Process

Every json file contains a wantErr flag to communicate to the consumer that this input should result in an error. Similarly, every input contains a desc field that describes what the test is attempting to validate. These two concepts were inconsistently used in the previous test suite so in an effort in standardize the json schema I adopted it everywhere.

Because the "desc" field is new to many of these tests, I had to do a bit of reverse engineering to figure out what each test input was trying to test. I'd encourage the reviewers here to validate that these additions are accurate. An example of this can be found in writeStaticConsistencyTestData. On that note, you'll notice some of these new descriptions are repetitive, because the test cases themselves are repetitive. I'd be happy to remove some of that repetition but I figured I'd defer to the maintainers before doing so.

A general issue with the test data that I believe some consumers may struggle with: a lot of these tests are (rightfully) designed for this specific implementation. Depending on the order in which a consumer performs these checks themselves, they may receive false negatives.

Lastly, a general note: I am not a Go developer, so feel free to suggest improvements wherever you see them 😄

Additional Changes

The first commit here removes a test case that was never referenced. The only reference to inclusionProofs skipped the first index because it was invalid.

Related Issue

Resolves: #169

Testing instructions

  1. Delete the testdata directory
  2. Run proofgen: go run ./cmd/proofgen
  3. Run tests in proof directory

@tannaurus tannaurus requested a review from a team as a code owner April 15, 2025 22:39
@tannaurus tannaurus requested a review from mhutchinson April 15, 2025 22:39
@google-cla
Copy link
Copy Markdown

google-cla bot commented Apr 15, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Copy Markdown
Contributor

@mhutchinson mhutchinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great work, thanks for doing this 🙏

I wanted to check this out locally, and found a couple of issues while doing so. I think they were non-contentious (fixing Fatal -> Fatalf, and deleting files that were no longer generated). LMK if you have issue though, I don't want to step on your toes!

I also added some bits to the presubmit check to make it easier to test that the generated files are still golden. I'll wire that up to github actions in a future PR though.

You'll need to accept the CLA to get this in. Hopefully that's not an issue.

I'll assign this to @AlCutter for final review now that I've left my fingerprints on this :-)

Copy link
Copy Markdown
Collaborator

@AlCutter AlCutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to send this PR!

I'm mostly just pointing out a few Go style bits along with a few suggestions.

I do worry about the special characters in some of the test data filenames (e.g. *, [, ]) - I think from the PoV of the code in here it's fine, but it's a potential "rake in the grass" for folks coming along later and hitting unexpected shell expansion issues. Perhaps we could use mul instead of * (similar to div you've used), and @<num> for the indices?

Ultimately it would be nice to have a README in here to help folks wanting to use the golden data to understand what's going on, clear descriptions of set up & tests, etc., but that's on us rather than you!

go run ./cmd/proofgen
fi

if [[ "${empty_diff}" -eq 1 ]]; then
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great addition. Thanks @mhutchinson !

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 274 lines in your changes missing coverage. Please review.

Project coverage is 57.69%. Comparing base (4ebea17) to head (bcc3f30).
Report is 71 commits behind head on main.

Files with missing lines Patch % Lines
cmd/proofgen/main.go 0.00% 274 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #171       +/-   ##
===========================================
- Coverage   89.33%   57.69%   -31.64%     
===========================================
  Files           7        8        +1     
  Lines         497      773      +276     
===========================================
+ Hits          444      446        +2     
- Misses         48      322      +274     
  Partials        5        5               

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AlCutter
Copy link
Copy Markdown
Collaborator

Thanks for the changes @tannaurus!

The linter has caught a bunch of errors which will need to be addressed before we can merge the PR, but I think this is a good step forward in making proofs more widely reusable.

@tannaurus tannaurus force-pushed the tg/write-golden-files branch from bcc3f30 to 848a337 Compare April 17, 2025 15:16
@AlCutter AlCutter force-pushed the tg/write-golden-files branch from 848a337 to ac550d5 Compare April 17, 2025 15:39
@AlCutter AlCutter merged commit bcba819 into transparency-dev:main Apr 17, 2025
10 of 12 checks passed
@AlCutter
Copy link
Copy Markdown
Collaborator

Thanks again @tannaurus!

@tannaurus
Copy link
Copy Markdown
Contributor Author

Thank YOU for the quick turn around on reviews 🙂 that was a fantastic open source experience. Feel free to mention that on a performance review 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make golden proof test data available to other languages

3 participants